postgres_fdw: Fix bug in checking of return value of PQsendQuery().

  • Jump to comment-1
    masao.fujii@oss.nttdata.com2022-07-21T14:22:26+00:00
    Hi, I found that fetch_more_data_begin() in postgres_fdw reports an error when PQsendQuery() returns the value less than 0 as follows though PQsendQuery() can return only 1 or 0. I think this is a bug. Attached is the patch that fixes this bug. This needs to be back-ported to v14 where async execution was supported in postgres_fdw. if (PQsendQuery(fsstate->conn, sql) < 0) pgfdw_report_error(ERROR, NULL, fsstate->conn, false, fsstate->query); Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
    • Jump to comment-1
      japinli@hotmail.com2022-07-21T14:41:54+00:00
      On Thu, 21 Jul 2022 at 22:22, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > Hi, > > I found that fetch_more_data_begin() in postgres_fdw reports an error when PQsendQuery() returns the value less than 0 as follows though PQsendQuery() can return only 1 or 0. I think this is a bug. Attached is the patch that fixes this bug. This needs to be back-ported to v14 where async execution was supported in postgres_fdw. > > if (PQsendQuery(fsstate->conn, sql) < 0) > pgfdw_report_error(ERROR, NULL, fsstate->conn, false, fsstate->query); > > Regards, +1. However, I think check whether the result equals 0 or 1 might be better. Anyway, the patch works correctly. $ grep 'PQsendQuery(' -rn . --include '*.c' ./contrib/postgres_fdw/postgres_fdw.c:7073: if (PQsendQuery(fsstate->conn, sql) < 0) ./contrib/postgres_fdw/connection.c:647: if (!PQsendQuery(conn, sql)) ./contrib/postgres_fdw/connection.c:782: if (!PQsendQuery(conn, query)) ./contrib/postgres_fdw/connection.c:1347: if (!PQsendQuery(conn, query)) ./contrib/postgres_fdw/connection.c:1575: if (PQsendQuery(entry->conn, "DEALLOCATE ALL")) ./contrib/dblink/dblink.c:720: retval = PQsendQuery(conn, sql); ./contrib/dblink/dblink.c:1146: if (!PQsendQuery(conn, sql)) ./src/test/isolation/isolationtester.c:669: if (!PQsendQuery(conn, step->sql)) ./src/test/modules/libpq_pipeline/libpq_pipeline.c:500: if (PQsendQuery(conn, "SELECT 1; SELECT 2") != 1) ./src/test/modules/libpq_pipeline/libpq_pipeline.c:532: if (PQsendQuery(conn, "SELECT 1.0/g FROM generate_series(3, -1, -1) g") != 1) ./src/test/modules/libpq_pipeline/libpq_pipeline.c:1000: if (PQsendQuery(conn, "SELECT 1") != 1) ./src/test/modules/libpq_pipeline/libpq_pipeline.c:1046: if (PQsendQuery(conn, "SELECT 1") != 1) ./src/test/modules/libpq_pipeline/libpq_pipeline.c:1084: if (PQsendQuery(conn, "SELECT 1") != 1) ./src/test/modules/libpq_pipeline/libpq_pipeline.c:1094: if (PQsendQuery(conn, "SELECT 2") != 1) ./src/test/modules/libpq_pipeline/libpq_pipeline.c:1118: if (PQsendQuery(conn, "SELECT 1") != 1) ./src/test/modules/libpq_pipeline/libpq_pipeline.c:1132: if (PQsendQuery(conn, "SELECT 2") != 1) ./src/test/modules/libpq_pipeline/libpq_pipeline.c:1159: if (PQsendQuery(conn, "SELECT pg_catalog.pg_advisory_unlock(1,1)") != 1) ./src/bin/pg_basebackup/pg_basebackup.c:1921: if (PQsendQuery(conn, basebkp) == 0) ./src/bin/pg_amcheck/pg_amcheck.c:891: if (PQsendQuery(slot->connection, sql) == 0) ./src/bin/psql/common.c:1451: success = PQsendQuery(pset.db, query); ./src/bin/scripts/reindexdb.c:551: status = PQsendQuery(conn, sql.data) == 1; ./src/bin/scripts/vacuumdb.c:947: status = PQsendQuery(conn, sql) == 1; ./src/bin/pgbench/pgbench.c:3089: r = PQsendQuery(st->con, sql); ./src/bin/pgbench/pgbench.c:4012: if (!PQsendQuery(st->con, "ROLLBACK")) ./src/backend/replication/libpqwalreceiver/libpqwalreceiver.c:663: if (!PQsendQuery(streamConn, query)) ./src/interfaces/libpq/fe-exec.c:1421:PQsendQuery(PGconn *conn, const char *query) ./src/interfaces/libpq/fe-exec.c:2319: if (!PQsendQuery(conn, query)) -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
      • Jump to comment-1
        masao.fujii@oss.nttdata.com2022-07-22T03:07:28+00:00
        On 2022/07/21 23:41, Japin Li wrote: > > On Thu, 21 Jul 2022 at 22:22, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: >> Hi, >> >> I found that fetch_more_data_begin() in postgres_fdw reports an error when PQsendQuery() returns the value less than 0 as follows though PQsendQuery() can return only 1 or 0. I think this is a bug. Attached is the patch that fixes this bug. This needs to be back-ported to v14 where async execution was supported in postgres_fdw. >> >> if (PQsendQuery(fsstate->conn, sql) < 0) >> pgfdw_report_error(ERROR, NULL, fsstate->conn, false, fsstate->query); >> >> Regards, > > +1. However, I think check whether the result equals 0 or 1 might be better. Maybe. I just used "if (!PQsendQuery())" style because it's used in postgres_fdw elsewhere. > Anyway, the patch works correctly. Thanks for the review! Pushed. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
        • Jump to comment-1
          etsuro.fujita@gmail.com2022-07-22T08:03:19+00:00
          On Fri, Jul 22, 2022 at 12:07 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > Pushed. This is my oversight in commit 27e1f1456. :-( Thanks for the report and fix, Fujii-san! Best regards, Etsuro Fujita
    • Jump to comment-1
      tgl@sss.pgh.pa.us2022-07-21T15:22:48+00:00
      Fujii Masao <masao.fujii@oss.nttdata.com> writes: > I found that fetch_more_data_begin() in postgres_fdw reports an error when PQsendQuery() returns the value less than 0 as follows though PQsendQuery() can return only 1 or 0. I think this is a bug. Attached is the patch that fixes this bug. This needs to be back-ported to v14 where async execution was supported in postgres_fdw. Yup, clearly a thinko. regards, tom lane